bounds-check packet code before indexing fr_radius_packet_name[]#5879
bounds-check packet code before indexing fr_radius_packet_name[]#5879aizu-m wants to merge 1 commit into
Conversation
|
We'll double-check and fix things. However, this is really a minor issue. The filter code is read from a local text file. So if you're passing garbage to radclient, that's your choice. |
|
Fair enough on severity, the value does come from a local file you control. The one thing worth flagging: the guard tests reply->code, but the index actually used is filter_code (and packet->code / filter_response_code in the others), so an in-range reply with an out-of-range Packet-Type still indexes past the 53-entry array. Not a trust boundary, agreed, just a guard checking a different variable than the one it is protecting. One-liner each, so I will leave it with you to fold in however you prefer. |
|
any update? |
|
We'll put in a slightly different patch. If the filter code is checked when the filter is read, there's no need to check the same filter code when receiving a packet. |
2 participants
fr_radius_packet_name[] has FR_RADIUS_CODE_MAX (53) entries:
Was reading the "Expected %s got %s" mismatch path in radclient. The
validity test guards reply->code, but the index that gets used is
filter_code:
filter_code is taken straight from the request/filter file as a uint32
(Packet-Type, vp->vp_uint32), so it is never range checked. A filter
whose Packet-Type is 200, with any in-range reply, reads 200 entries
into a 53-entry array and hands the result to %s.
recv_one_packet() and radclient-ng client_read() have the same shape.
The radsniff startup banner prints filter_request_code and
filter_response_code (also Packet-Type values), guarded only against
zero.
Each guard now validates the code that is actually used as the index.
Behaviour is unchanged for valid codes; out-of-range codes fall through
to the numeric branch, or to "unknown" in the radsniff banner.